-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ExtensionContext on instance creation #4032
base: main
Are you sure you want to change the base?
Conversation
see #3445 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduced the annotation @EnableTestScopedConstructorContext
to enable the new behavior for TestInstancePreConstructCallback
, TestInstancePostProcessor
and TestInstanceFactory
. I have few remaining questions, which I have added as review comments.
@Target(ElementType.TYPE) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Inherited | ||
@API(status = MAINTAINED, since = "5.12") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure which status to use in the API
annotation. Which status do you suggest?
If we want to us EXPERIMENTAL
for now, then I should probably also reformulate my note in the Javadoc. (Suggesting to switch to an experimental API for future compatibility seems a bit odd. 😄)
@Retention(RetentionPolicy.RUNTIME) | ||
@Inherited | ||
@API(status = MAINTAINED, since = "5.12") | ||
public @interface EnableTestScopedConstructorContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to suggest other names. This is the best name that came to mind. I think the name is good enough, at least I like it much more than my other ideas (e.g. NewTestInstanceConstructionContext
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that you have already suggested @MethodLevelExtensionContextAware
. I don't have a strong preference. Your suggestion is a bit shorter.
private Object invokeTestClassConstructor(Optional<Object> outerInstance, ExtensionRegistry registry, | ||
ExtensionContext extensionContext) { | ||
ExtensionContext extensionContext, ExtensionContext ourExtensionContext) { | ||
|
||
Constructor<?> constructor = ReflectionUtils.getDeclaredConstructor(this.testClass); | ||
return executableInvoker.invoke(constructor, outerInstance, extensionContext, registry, | ||
InvocationInterceptor::interceptTestClassConstructor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calls to ParameterResolver
are not yet looking at the new annotation. Shall calls to ParameterResolver
also consider the annotation?
I just wanted to confirm that I should adjust this method as well. I was a bit reluctant to change InterceptingExecutableInvoker
for some reason. For now, I wrote the documentation matching the current behavior, I will change the documentation together with the implementation.
if (AnnotationUtils.isAnnotated(this.testInstanceFactory.getClass(), | ||
EnableTestScopedConstructorContext.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little bit concerned about performance with all these calls to AnnotationUtils.isAnnotated
. Do you think this could be an issue?
Overview
Resolve #1568.
Resolve #2970.
Resolve #3445.
This PR moves the instance creation into the execution context of the test method. This affects the following callbacks:
TestInstancePreConstructCallback
TestInstanceFactory
ParameterResolver
(when resolving parameters for the constructor)TestInstancePostProcessor
Assuming the test is using
TestInstance.Lifecycle.PER_METHOD
(the default), these callbacks will now receive theExtensionContext
for the currently executed test method. This has the following effects:ExtensionContext.getTestMethod()
ExtensionContext.getTestClass()
andTestInstanceFactoryContext.getTestClass()
are no-longer interchangeableExtensionContext.getTestClass()
now returns the (nested) class of the currently executed testCloseableResource
which are added to the store will now be closed at the end of the test executionIf the test is using
TestInstance.Lifecycle.PER_CLASS
, the behavior is unaltered.Not sure if the change is considered API breaking. The previous behavior was rather unintuitive and not explicitly documented. However, the example at
TestInfoDemo
is no-longer valid and has been changed as part of the PR. I also think there is a good chance that there are a few extensions in the wild which rely on the previous behavior, since the behavior was in place for multiple years. I would therefore like to know how I should proceed with the PR.I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@API
annotations